fix(clob): type balance-allowance allowances as HashMap<Address, U256>#329
Open
moeuu wants to merge 1 commit intoPolymarket:mainfrom
Open
fix(clob): type balance-allowance allowances as HashMap<Address, U256>#329moeuu wants to merge 1 commit intoPolymarket:mainfrom
moeuu wants to merge 1 commit intoPolymarket:mainfrom
Conversation
The `/balance-allowance` endpoint returns the `allowances` map as
`{address: "<uint256 decimal string>"}`. For MetaMask-logged-in
Polymarket accounts the typical "fully approved" state — reached the
first time the UI triggers `setApprovalForAll` plus USDC `approve` on
the exchange contracts — returns `2^256 - 1` as each value:
"allowances": {
"0x4bfb41d5b3570defd03c39a9a4d8de6bd8b8982e":
"115792089237316195423570985008687907853269984665640564039457584007913119639937",
...
}
The previous type was `HashMap<Address, String>`, which forced every
consumer to re-parse the values. The obvious parse path (mirroring the
`balance: Decimal` field directly above it) is
`rust_decimal::Decimal::from_str(v)`, which *silently drops* every
fully-approved entry because `2^256 - 1 ≈ 1.15e77` is far above
`Decimal::MAX ≈ 7.9e28`. A downstream allowance-sum helper that
filtered with `filter_map(|v| v.parse::<Decimal>().ok())` would then
report "no allowances" for users who are, in fact, fully approved,
blocking order submission or making approvals-check lie.
Changing the field type to `HashMap<Address, U256>` — which is the
honest on-chain type — fixes this class of bug at the SDK boundary.
`U256` already has `serde::Deserialize` support via alloy, so the
JSON-string-to-U256 parse happens for free during deserialization,
and consumers receive a correctly-typed value that can safely hold
MAX_UINT256. This is a breaking API change: downstream code that
accessed `.allowances: HashMap<Address, String>` directly will need
to update the type.
Regression test `balance_allowance_accepts_max_uint256_allowances`
feeds a real MAX_UINT256 literal through an httpmock server and
asserts the deserialized value equals `U256::MAX`, which locks in the
expected behavior.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #329 +/- ##
=======================================
Coverage 85.54% 85.54%
=======================================
Files 32 32
Lines 5167 5167
=======================================
Hits 4420 4420
Misses 747 747
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
balance-allowancereturns MAX_UINT256 allowances that overflowrust_decimal::DecimalSummary
The CLOB
/balance-allowanceendpoint returns theallowancesmap asHashMap<Address, String>, where each value is a decimal string representing auint256on-chain allowance. When a user has fully approved the Polymarket CTFExchange (the normal state after the first UI trade triggers
setApprovalForAll),the value is
2^256 - 1:{ "balance": 58916378, "allowances": { "0x4bfb41d5b3570defd03c39a9a4d8de6bd8b8982e": "115792089237316195423570985008687907853269984665640564039457584007913119639937", "0xc5d563a36ae78145c45a50134d48a1215220f80a": "115792089237316195423570985008687907853269984665640564039457584007913122515535", "0xd91e80cf2e7be2e162c6513ced06f1dd0da35296": "115792089237316195423570985008687907853269984665640564039457584007913129639935" } }Any downstream consumer that tries to parse these strings as
rust_decimal::Decimal(the natural choice, matching the rest of the API) silently fails for every
entry because
2^256 - 1 ≈ 1.15e77is far aboveDecimal::MAX ≈ 7.9e28.Concretely, a helper like this silently drops every entry:
so approval-status checks report "no allowances" even though the user has
actually fully approved all three exchange contracts. The downstream consumer
either erroneously blocks orders or has to build an ad-hoc overflow-saturating
parser as a workaround.
Proposed fix
This is a semantic issue (Decimal is the wrong type), not a bug in the SDK's
network layer. There are two reasonable paths:
Option A: type
allowancesasHashMap<Address, U256>The SDK already uses
alloy_primitives::U256elsewhere (e.g. inSignedOrder,token_id, etc.), and the API value is morphologically auint256on-chain allowance, so this is the "honest" type:Breaking change, but the direction is clearly correct, and downstream code
that currently parses
.value.parse::<U256>()can drop the parse step.Option B: keep
Stringand document the overflow trap prominentlyLess disruptive but punts the problem to every caller. If this is chosen, I
suggest adding a module-level note on
BalanceAllowanceResponseplus anexample in the docs showing the recommended
U256::from_str_radix(v, 10)parse path.
I'd lean toward A given that infinite approvals are the norm on Polymarket
post-UI-trade and every live user hits this.
Follow-on:
balancefieldWhile you're there,
balancehas the same flavor — it's auint256with 6decimal fixed-point semantics (USDC). A user balance of
$58.916378arrives as"58916378". Decimal handles this fine because the raw value fits, but if thecollateral ever grows to millions of USDC represented in 18-decimal fixed
point, it would also overflow. Switching to
U256uniformly would preempt this.Reproduction
Live example captured during authenticated
balance_allowance(Collateral)onan MetaMask-logged-in account that has traded through the Polymarket UI.
Notes
polymarket-client-sdk v0.4.4.DecimalFromAnydeserializer fix in a separate PR.Note
Medium Risk
This is a breaking API-type change (
allowancesvalues move fromStringtoU256) that can impact downstream consumers and serialization assumptions. Runtime risk is otherwise low since it primarily affects deserialization of the/balance-allowanceresponse.Overview
Updates
BalanceAllowanceResponse.allowancestoHashMap<Address, U256>(fromString) to correctly represent on-chainuint256allowances and avoid overflow/consumer re-parsing when the backend returns2^256 - 1.Adjusts CLOB integration tests accordingly and adds a regression test ensuring
/balance-allowanceaccepts and deserializesMAX_UINT256allowances toU256::MAX.Reviewed by Cursor Bugbot for commit 0140d9a. Bugbot is set up for automated code reviews on this repo. Configure here.